Skip to content
This repository has been archived by the owner on Jan 30, 2024. It is now read-only.

Move back to HDBC #34

Closed
wants to merge 6 commits into from
Closed

Move back to HDBC #34

wants to merge 6 commits into from

Conversation

viviag
Copy link
Contributor

@viviag viviag commented Sep 8, 2018

This branch is intended to be stable version of this fork.
There is no hasql since it does not support GHC-8.4.
Also there is working test suite.

TODO:

  • Enable CI
  • Use more efficient types (version in master was really fast)

@viviag viviag closed this Sep 8, 2018
@viviag
Copy link
Contributor Author

viviag commented Sep 8, 2018

Sorry. I accidentally compared with wrong branch.

@jtdaugherty
Copy link
Owner

Okay, thanks for the clarification; I was confused. Are you intending to submit a pull request in the future, though?

@viviag
Copy link
Contributor Author

viviag commented Sep 8, 2018

Hope so if you want this work to be part of the project.
I mean just sensible parts; Form will be changed a lot.
These sensible changes:

  • Converting to Stack project.
  • Storing migrations with yml extension.
  • Cleaning up dependencies.
    And goods mentioned in initial PR comment, I guess.

@jtdaugherty
Copy link
Owner

Converting to Stack project.

I don't support Stack on any of my projects. You are welcome to use Stack to build it, obviously, but I will not accept any patches containing Stack package files.

Storing migrations with yml extension.

This breaks existing installations. Does your patch deal with compatibility?

Cleaning up dependencies.

Can you be more specific?

And goods mentioned in initial PR comment, I guess.

Can you be more specific here, too? Do you mean Travis? And as for efficiency, details would be helpful there, too.

Thanks!

@viviag
Copy link
Contributor Author

viviag commented Sep 8, 2018

This breaks existing installations. Does your patch deal with compatibility?

Is this patch acceptable at all?
It can become compatible if allow to read .txt along with .yml.

Cleaning up dependencies.

Can you be more specific?

Sorry. As I see there is no dependencies changed against master. Of course commit messages will be more adequate in real PR.

Can you be more specific here, too? Do you mean Travis? And as for efficiency, details would be helpful there, too.

I mean Travis or Circle.

About efficiency: I forked the project first to learn hasql on practice. Following database mapping engine change I went through the code and replaced String with ByteString. And it became essentially faster.
I do not propose to use hasql right now; There is no more hasql dependency in the fork. But I still believe there had happened a lot in ecosystem to check, particularly in sense of performance.

I didn't expect it to be part of origin ever but I'm ready to work on it in sense of this discussion.

@jtdaugherty
Copy link
Owner

Is this patch acceptable at all?
It can become compatible if allow to read .txt along with .yml.

I would be happy to accept a patch that worked for either extension. So, to be specific, when the tool creates new migrations it can create them with a yml extension, but still look for txt files too, for compatibility.

Also, the patch for this should also document the behavior in the relevant places, especially MOO.TXT.

I mean Travis or Circle.

I prefer Travis, so I'd be happy to accept a patch that sets that up.

As far as performance goes, ByteString works for me.

Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants